Skip to content

[FEAT] Displays decimal amount #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fermuch merged 2 commits into from
Feb 27, 2025
Merged

[FEAT] Displays decimal amount #13

fermuch merged 2 commits into from
Feb 27, 2025

Conversation

@Vinocis
Copy link
Collaborator

@Vinocis Vinocis commented Feb 27, 2025 •
edited by github-actions bot
Loading

User description

Now the Numscriptex.Posting{} and Numscriptex.Balance{} also displays the decimal amount/balances for a better user experience.

PR Type

Enhancement, Tests, Documentation

Description

  • Added decimal representation for balances and postings.

  • Introduced utility functions for decimal conversions.

  • Updated tests to cover new decimal fields.

  • Enhanced documentation with examples of decimal usage.

Changes walkthrough 📝

Relevant files
Enhancement
balance.ex
Add decimal fields and conversion in Balance module           

lib/numscriptex/balance.ex

  • Added decimal_final_balance and decimal_initial_balance fields.
  • Implemented put_decimal_values function for decimal conversion.
  • Updated type specifications and documentation.
    		• +48/-14 
    posting.ex
    Add decimal field and conversion in Posting module             

    lib/numscriptex/posting.ex

  • Added decimal_amount field to postings.
  • Implemented put_decimal_amount function for conversion.
  • Updated type specifications and documentation.
    		• +23/-4   
    utilities.ex
    Add utility functions for decimal conversion                         

    lib/numscriptex/utilities.ex

  • Added integer_to_decimal function for conversion.
  • Added decimal_places_from_asset function.
  • Documented new utility functions with examples.
    		• +60/-0   
    Tests
    balance_test.exs
    Update Balance tests for decimal fields                                   

    test/numscriptex/balance_test.exs

  • Updated tests to include decimal balance fields.
  • Verified decimal conversion accuracy in tests.
    		• +9/-3     
    posting_tests.exs
    Update Posting tests for decimal field                                     

    test/numscriptex/posting_tests.exs

  • Updated tests to include decimal amount field.
  • Verified decimal conversion accuracy in tests.
    		• +5/-2     
    numscriptex_test.exs
    Update integration tests for decimal fields                           

    test/numscriptex_test.exs

  • Updated integration tests to include decimal fields.
  • Verified decimal conversion accuracy in integration tests.
    		• +197/-53
    Documentation
    README.md
    Update README with decimal field examples                               

    README.md

  • Updated examples to include decimal fields.
  • Enhanced documentation with decimal usage.
    		• +11/-4   
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @Vinocis Vinocis requested a review from fermuch February 27, 2025 15:56
    @Vinocis Vinocis self-assigned this Feb 27, 2025
    @github-actions
    Copy link

    github-actions bot commented Feb 27, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The function decimal_places_from_asset assumes that the asset string length is at least 5 to extract decimal places. This might not be valid for all asset strings, potentially leading to incorrect default values being used.

     
    def decimal_places_from_asset(asset, default \\ 2) do
  if String.length(asset) >= 5 do
    asset
    |> String.last()
    |> String.to_integer()
  else
    default
  end
end

    @github-actions
    Copy link

    github-actions bot commented Feb 27, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add validation for asset format

    Ensure that the decimal_places_from_asset function correctly handles assets with
    unexpected formats by adding validation or error handling for cases where the asset
    string does not conform to expected patterns.

    lib/numscriptex/utilities.ex [97-105]

     def decimal_places_from_asset(asset, default \\ 2) do
-  if String.length(asset) >= 5 do
+  if String.length(asset) >= 5 and String.contains?(asset, "/") do
     asset
-    |> String.last()
+    |> String.split("/")
+    |> List.last()
     |> String.to_integer()
   else
     default
   end
 end
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion improves the robustness of the decimal_places_from_asset function by ensuring it handles unexpected asset formats more gracefully. This change can prevent potential runtime errors when the asset string does not conform to expected patterns, enhancing the function's reliability.

    		Medium

    fermuch
    fermuch requested changes Feb 27, 2025
    View reviewed changes
    fermuch
    fermuch approved these changes Feb 27, 2025
    View reviewed changes
    Copy link
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM

    @fermuch fermuch merged commit d1d1bfa into main Feb 27, 2025
    4 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @fermuch fermuch fermuch approved these changes

    Assignees

    @Vinocis Vinocis

    Labels

    Review effort 3/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @Vinocis @fermuch